Add global --quiet, --no-input, and --yes flags#4726
Add global --quiet, --no-input, and --yes flags#4726simonfaltum wants to merge 12 commits intomainfrom
Conversation
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
|
Commit: 84b9225
18 interesting tests: 9 SKIP, 7 KNOWN, 2 flaky
Top 20 slowest tests (at least 2 minutes):
|
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
requested review from
a team,
andrewnester,
anton-107,
denik,
jefferycheng1,
kanterov,
lennartkats-db,
pietern and
shreyas-goenka
as code owners
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
shreyas-goenka
left a comment
shreyas-goenka
left a comment
There was a problem hiding this comment.
Note: This review was posted by Claude (AI assistant). Shreyas will do a separate, more thorough review pass.
Priority: LOW-MEDIUM — Strong implementation with minor issues
LOW: Description casing inconsistency
Flag descriptions use inconsistent casing (some start with uppercase, others lowercase). Minor but worth standardizing.
LOW: tools/gofumpt artifact
There appears to be a formatting change from tools/gofumpt mixed into this PR. Consider separating formatting-only changes.
What looks good
- Clean implementation of
--quiet,--no-input, and--yesflags - Good integration with existing
cmdiocapabilities system - Proper propagation through the command tree
- Well-structured capability checks
- Correct interaction between the three flags
Overall a solid PR. The issues are minor.
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
libs/cmdio/io.go
Outdated
| teaProgram: c.teaProgram, | ||
| teaDone: c.teaDone, |
There was a problem hiding this comment.
we're shallow cloning teaProgram but not reusing its mutex. A cloned cmdIO instance with a different mutex can race on teaProgram and teaDone with the original
There was a problem hiding this comment.
Good eye on the mutex, but I think this is safe as-is. clone() is only called from withCapabilities(), which is only called from SetQuiet/SetNoInput/SetYes. These run once during flag initialization in PersistentPreRunE, before any spinner or Bubble Tea program is ever started. At that point both teaProgram and teaDone are nil.
After the clone, the new cmdIO replaces the old one in the context (InContext(ctx, c)), and nobody holds a reference to the old instance anymore. So there's no window where two cmdIO instances with separate mutexes are concurrently operating on the same tea state.
That said, copying nil fields that we never actually need is a bit misleading. I'll remove the teaProgram and teaDone lines from clone() to make the intent clearer (they're always nil at clone time, so no behavioral change).
shreyas-goenka
left a comment
shreyas-goenka
left a comment
There was a problem hiding this comment.
re: --quiet. We already have the --log-level flag. Worth looking into whether we should repurpose it or a new flag is warrented.
re: --no-input, what's the usecase? Today the CLI dynamically detects whether interactive input is possible and errors when that is not supported (like in CI)
re: --yes. We use the --auto-approve flag in other contexts. Let's not add a new --yes flag.
Add three new global flags to control CLI interaction behavior in CI/CD pipelines and scripts: - --quiet/-q (DATABRICKS_QUIET): suppresses non-essential output (spinners, LogString/Log calls, non-error diagnostics) - --no-input (DATABRICKS_NO_INPUT): disables all interactive prompts, returning ErrNoInput or default values where available - --yes/-y (DATABRICKS_YES): auto-approves yes/no confirmation prompts Precedence for AskYesOrNo: --yes > --no-input > normal prompting. Non-interactive guards in deploy, destroy, and completion install are updated to also accept --yes as an alternative to --auto-approve. Co-authored-by: Isaac
…, test cleanup Co-authored-by: Isaac
Add --no-input, --quiet, and --yes to all help output golden files. Update error messages to mention --auto-approve or --yes. Co-authored-by: Isaac
Use env.GetBool for the global interaction flags and derive new cmdio contexts when quiet, no-input, or yes is enabled so commands do not mutate shared I/O state.
…ands Fix four issues with the global interaction flags: 1. configure: check IsNoInput alongside IsPromptSupported so --no-input correctly routes to the non-interactive path. 2. auth prompts: add IsNoInput checks to promptForHost, promptForAccountID, and promptForWorkspaceID so --no-input prevents raw promptui prompts. 3. logout: allow --yes to bypass the prompt-supported guard so AskYesOrNo can auto-approve in non-interactive environments. 4. apps import: remove local --quiet flag that shadowed the root persistent --quiet, and read from cmdio.IsQuiet(ctx) instead. Also revert the unrelated tools/gofumpt addition to .gitignore.
promptForProfile (login), resolveProfileAmbiguity (bundle), and the logout profile picker all check IsPromptSupported but did not check IsNoInput. On an interactive terminal with --no-input these functions would still show a prompt. Add the missing guard. Also rephrase the logout error from "non-interactive environment" to "prompting is disabled" so the message is accurate for both non-interactive terminals and explicit --no-input usage. Co-authored-by: Isaac
In quiet mode, skip the allocation of errorsOnly slice when there are no errors. Add a doc comment to Prompt() noting that callers must check IsNoInput themselves. Co-authored-by: Isaac
Co-authored-by: Isaac
These fields are always nil at clone time (clone only runs during flag init, before any Bubble Tea program starts). Removing them makes it clear the clone does not share tea lifecycle state with the original. Co-authored-by: Isaac
simonfaltum
force-pushed
the
simonfaltum/global-interaction-flags
branch
from
eab7952 to
84b9225
Compare
simonfaltum
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Suggested reviewersBased on git history of the changed files, these people are best suited to review:
Confidence: high Eligible reviewersBased on CODEOWNERS, these people or teams could also review: @andrewnester, @anton-107, @databricks/eng-apps-devex, @jefferycheng1, @kanterov, @lennartkats-db, @shreyas-goenka Suggestions based on git history of 62 changed files (19 scored). See CODEOWNERS for path-specific ownership rules. |
pietern
left a comment
pietern
left a comment
There was a problem hiding this comment.
I see !cmdio.IsPromptSupported(ctx) || cmdio.IsNoInput(ctx) at many call sites. This indicates to me that they should be combined into a single thing.
The --yes and -q flags are different from --no-input and deserve different PRs with discussion in my opinion. The only thing they share is centralized storage in cmdio.
| cmd.Flags().StringVar(&outputDir, "output-dir", "", "Directory to output the bundle to (defaults to app name)") | ||
| cmd.Flags().BoolVar(&cleanup, "cleanup", false, "Clean up the previous app folder and all its contents") | ||
| cmd.Flags().BoolVar(&forceImport, "force-import", false, "Force re-import of an app that was already imported (only works for apps you own)") | ||
| cmd.Flags().BoolVarP(&quiet, "quiet", "q", false, "Suppress informational messages (only show errors and prompts)") |
There was a problem hiding this comment.
The "(only show errors and prompts)" makes me think; what is the policy for -q?
The same?
| -q, --quiet Suppress non-essential output | ||
| -t, --target string bundle target to use (if applicable) | ||
| --var strings set values for variables defined in bundle config. Example: --var="foo=bar" | ||
| -y, --yes Auto-approve all yes/no prompts |
There was a problem hiding this comment.
The other flags use lower sentence case.
| // SupportsPrompt returns true if terminal supports user prompting. | ||
| // Prompts write to stderr and read from stdin, so we only need those to be TTYs. | ||
| // Note: this only reflects terminal capability. The --no-input flag is checked | ||
| // directly in prompt functions (Ask, AskYesOrNo, etc.) rather than here. |
There was a problem hiding this comment.
If this is the case, it should be a separate type.
Or otherwise, --no-input should be folded into cmdio.IsPromptSupported().
It could return (ok bool, reason string) for error messages if we need to differentiate (looking at a few call sites I think we do, eg "cannot prompt" vs "prompt disabled via --no-input", or "prompt disabled via DATABRICKS_NO_INPUT").
|
It would good to align on behaviour first:
I think first we should introduce a log level for LogString(), e.g. NOTICE: WARN > NOTICE > INFO. Not everything that is currently a LogString would become NOTICE though, some things, like delete or recreate messages should be warnings. Then I'd expect -q to drop to one level: silence NOTICE, but keep WARN. Likewise -v would include one more level: +INFO (currently not shown). -vv would also add DEBUG. -vvv: TRACE |
|
|
||
| Global Flags: | ||
| --debug enable debug logging | ||
| --no-input Disable interactive prompts |
There was a problem hiding this comment.
Duplicate of --auto-approve?
| --no-input Disable interactive prompts | ||
| -o, --output type output type: text or json (default text) | ||
| -p, --profile string ~/.databrickscfg profile | ||
| -q, --quiet Suppress non-essential output |
There was a problem hiding this comment.
Suppress too much. Some of what goes through LogString is essential.
| -q, --quiet Suppress non-essential output | ||
| -t, --target string bundle target to use (if applicable) | ||
| --var strings set values for variables defined in bundle config. Example: --var="foo=bar" | ||
| -y, --yes Auto-approve all yes/no prompts |
There was a problem hiding this comment.
Another duplicate of --auto-approve?
6 participants
Why
CLI users in CI/CD pipelines and scripts need control over the CLI's interactive behavior. Currently there's no way to suppress progress output, prevent prompts from blocking automation, or auto-approve confirmations without per-command flags.
Changes
Before: No global flags for controlling output verbosity, interactivity, or confirmations. Scripts had to work around interactive prompts.
Now: Three new global flags on the root command, stored in the cmdio Capabilities struct alongside existing TTY detection:
--quiet/-q(env:DATABRICKS_QUIET): Suppresses non-essential output. Spinners become no-ops,LogString/Logcalls are silenced, and only Error-severity diagnostics render. Data output (stdout) is never affected.--no-input(env:DATABRICKS_NO_INPUT): Disables all interactive prompts. Prompt functions returnErrNoInput(or the default value if one exists).--yes/-y(env:DATABRICKS_YES): Auto-approves all yes/no confirmation prompts. Precedence: --yes (approve) > --no-input (error) > normal prompting. Non-interactive guards in deploy/destroy/completion/bind are updated to also check --yes.apps importalready had its own local-q/--quietflag that suppresses informational messages during import. The two flags are bridged: passing the global--quietalso sets the local quiet behavior in apps import, and passing the local-qon apps import also activates the global cmdio quiet mode (suppressing spinners, etc.). Both flags continue to work as before.Post-review fixes
promptForProfileincmd/auth/login.goon--no-input(was only checkingIsPromptSupported)cmd/root/bundle.goon--no-input--no-input)HasError()early return inRenderDiagnosticsquiet mode to avoid unnecessary allocationPrompt()noting callers must checkIsNoInput(ctx)before callingRun()Open items
quiet,no_input,yesshould be added to a global reject list for top-level field names in auto-generated API commands, so future API parameters don't collide with these global flags. This needs a change in the codegen pipeline (or universe API linters), tracked separately.cmdio(LogString, Log, Render, spinners). A follow-up could add a lint rule or test verifying no directfmt.Fprintf(os.Stderr, ...)calls bypass cmdio, so--quietis truly global.--no-inputaudit: ~25 otherIsPromptSupportedcall sites in the codebase don't check--no-input. A follow-up could foldnoInputintoSupportsPrompt()so all prompt paths automatically respect--no-input.Test plan
make lintfullpassesmake checkspasses